-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sundry fixes #747
Sundry fixes #747
Conversation
searchFn(query) | ||
setShowResults(true) | ||
} | ||
// Properly adding all dependancies would require transforming `searchFn` into a callback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from phone, but something like:
const searchFnRef = useRef(searchFn)
searchFnRef.current = searchFn
useEffect(
...searchFnRef.current(),
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dunno if that's better... :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disabling the eslint rule is unsafe for future changes which can more easily introduce bugs.. mutable refs might not look good, but you explicitly say "this is not a dependency which triggers a synchronization, I just want to use whatever value is has currently" without implying anything about any other variable that may or may not be a dependency
@@ -25,4 +25,4 @@ MATOMO_DOMAIN = "{MATOMO_DOMAIN}" | |||
DISCORD_LOGGING_CHANNEL_ID = "{DISCORD_LOGGING_CHANNEL_ID}" | |||
DISCORD_LOGGING_TOKEN = "{DISCORD_LOGGING_TOKEN}" | |||
EDITOR_USERNAME = "{EDITOR_USERNAME}" | |||
EDITOR_PASSWORD = "{EDITOR_PASSWORD}" | |||
EDITOR_PASSWORD = "{EDITOR_PASSWORD}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing whitespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, why removing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know about the code, but the format to request the pages looks good.
e.g.